Skip to content

Conversation

Nols1000
Copy link

@Nols1000 Nols1000 commented Dec 31, 2024

This allows for a better configuration in case the server restricts the maximum query depth.


Added by @benjie:

…ery depth

This allows for a better configuration in case the server restricts the maximum query depth.
@Nols1000 Nols1000 requested a review from a team as a code owner December 31, 2024 14:19
Copy link

linux-foundation-easycla bot commented Dec 31, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

benjie
benjie previously requested changes Jan 3, 2025
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this nicely formatted :)

@benjie benjie changed the title Update getIntrospectionQuery.ts to allow configuration of the type query depth Update getIntrospectionQuery.ts to allow configuration of the type introspection depth Jan 3, 2025
@benjie benjie changed the title Update getIntrospectionQuery.ts to allow configuration of the type introspection depth Allow configuration of the ofType introspection depth Jan 3, 2025
@Nols1000
Copy link
Author

Nols1000 commented Jan 3, 2025

@benjie Thanks for the suggestions. I've commited them my branch. I think for the scope of this PR that is the right move.

I'd also like to take this as an opportunity to discuss formatting the introspection query. I think it would be simpler to just keep the query in one line, without indentions. This could clean up the code and save a few byte over the wire. It would of course reduce the readability. Maybe we can document the query better, so that would become less of an issue.

@benjie
Copy link
Member

benjie commented Jan 5, 2025

You can always minify it by parsing it and using a GraphQL minifier. I don't think our source code should concern itself with that too much - we should make it easy to read and edit. What sort of comments do you have in mind?

@ThePlenkov
Copy link

@Nols1000 can you run please npm run lint -- --fix in your branch to get rid of a lint error?

@ThePlenkov
Copy link

@Nols1000 also please cover this line with a test case:
image

You can see it here https://app.codecov.io/gh/graphql/graphql-js/pull/4317

Thanks!

I decided not to submit identical MR, so now also interested in introducing this change

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good feature to have, great work, left some comments but pending those changes this should be good to go

@Nols1000
Copy link
Author

@benjie At least for me it's not 100% clear how the query works. I would have expected more documentation e.g. here https://graphql.org/learn/introspection/. For me a breakdown of this introspection query would have been a great learning resource.

@JoviDeCroock JoviDeCroock added the PR: feature 🚀 requires increase of "minor" version number label Jan 16, 2025
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce introspection complexity programmatically
5 participants